Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase apparent test coverage by removing superfluous ellipses #142

Open
wants to merge 1 commit into
base: enh/run-clone
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

Yesterday I learned that one does not need to put any code inside a function that has a docstring. This allows us to remove numerous lines in the abstract layer that just contain ... and are counted by test coverage as missing lines. Removing them should not affect any kind of behaviour or how the docs look, but increase the apparent test coverage :)

@glatterf42 glatterf42 added the enhancement New feature or request label Dec 6, 2024
@glatterf42 glatterf42 self-assigned this Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.1%. Comparing base (0975ce3) to head (64d7787).

Additional details and impacted files
@@               Coverage Diff               @@
##           enh/run-clone    #142     +/-   ##
===============================================
+ Coverage           87.1%   88.1%   +0.9%     
===============================================
  Files                231     231             
  Lines               8216    8125     -91     
===============================================
  Hits                7160    7160             
+ Misses              1056     965     -91     
Files with missing lines Coverage Δ
ixmp4/data/abstract/docs.py 100.0% <ø> (+21.0%) ⬆️
ixmp4/data/abstract/iamc/datapoint.py 98.4% <ø> (+5.8%) ⬆️
ixmp4/data/abstract/iamc/timeseries.py 84.3% <ø> (+13.3%) ⬆️
ixmp4/data/abstract/iamc/variable.py 96.1% <ø> (+12.8%) ⬆️
ixmp4/data/abstract/meta.py 98.6% <ø> (+8.7%) ⬆️
ixmp4/data/abstract/model.py 96.1% <ø> (+12.8%) ⬆️
ixmp4/data/abstract/optimization/equation.py 96.9% <ø> (+16.9%) ⬆️
ixmp4/data/abstract/optimization/indexset.py 96.2% <ø> (+15.0%) ⬆️
ixmp4/data/abstract/optimization/parameter.py 96.8% <ø> (+15.2%) ⬆️
ixmp4/data/abstract/optimization/scalar.py 97.1% <ø> (+14.2%) ⬆️
... and 7 more

@meksor
Copy link
Contributor

meksor commented Dec 17, 2024

Hmm, so what you are saying does check out. The ellipses is not really strictly required.

from typing import Protocol

class A(Protocol):
    def f(self):
        """This is a docstring"""
        ...

class B(A): ...

B().f()
# ^ Cannot instantiate abstract class "B" with abstract attribute "f"

And alike:

from typing import Protocol

class A(Protocol):
    def f(self):
        """This is a docstring"""

class B(A): ...

B().f()
# ^ Cannot instantiate abstract class "B" with abstract attribute "f"

However as soon as A.fcontains a yield or return the mypy error disappears, so it might be worth leaving these few lines so it doesnt happen accidentally.

@glatterf42
Copy link
Member Author

I'm not sure I understand the problem with this. I created your example in the mypy playground, so please feel free to experiment with that.
The PR only removes ... from functions where this is the only "body" of the function. Whenever a function contains yield or return, it remains unchanged. There is no intersection of yield/return and ... in functions, it's either or. And the error is function-specific, as my playground example shows: B().f() doesn't work even if f() has a return statement if g() is abstract. So with or without the ellipses, one could only call the yield/return functions on child classes that actually provide implementations for the abstract functions.

Please let me know what I'm missing about this.

@glatterf42 glatterf42 force-pushed the enh/remove-superfluous-ellipses branch from f996fb5 to 64d7787 Compare December 20, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants